-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Feature: MPEG-5 LCEVC Scalable support #4572
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature: MPEG-5 LCEVC Scalable support #4572
Conversation
Updated references to track to representation
Removed section on shorter buffers for demoing purposes
|
@v-nova-romas Thank you for your PR. Can you please merge the latest changes from |
|
Hi @dsilhavy Thank you, I have merged the latest changes |
…vcScalableSupport
src/dash/vo/Representation.js
Outdated
| this.startNumber = 1; | ||
| this.timescale = 1; | ||
| this.width = NaN; | ||
| this.dependentRepresentation = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all member attributes in this constructor are sorted alphabetically.
Can you move this new entry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @stschr Thank you for taking the time to review the PR!
the new entry should now be sorted alphabetically
src/streaming/constants/Constants.js
Outdated
| COMMON_ACCESS_TOKEN_HEADER: 'common-access-token', | ||
| DASH_ROLE_SCHEME_ID : 'urn:mpeg:dash:role:2011', | ||
|
|
||
| ENHANCEMENT_CODECS: ['lvc1'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this code, 'lvc1' gets always considered as supported by src/streaming/utils/Capabilities.js#L141
However, shouldn't we reject this 4CC if lcevc_dec.js is not registered as extension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, agreed! I would like to modify Capabilities.js#L141 to also check if lcevc_dec.js is loaded, and reject otherwise.
May I ask for clarification regarding registered as extension - is there a way in dash.js to register lcevc_dec.js as a loaded extension? I could check if window.LCEVCdec is defined but I am wondering if there is a more idiomatic way.
src/streaming/models/CmcdModel.js
Outdated
| ot = CmcdObjectType.AUDIO; | ||
| } | ||
| if (request.mediaType === Constants.ENHANCEMENT) { | ||
| ot = 'e'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe not an issue, but where does this magic constant string come from? Is it allowed by Cmcd-spec? (@dsilhavy )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally we have considered that the enhancement doesn't quite fit under existing Cmcd Object Types. However, after looking at the Cmcd-spec it seems that tokens not defined in the spec (eg. e) would be discarded by a Cmcd server.
I would like to assign the enhancement ot to CmcdObjectType.OTHER. Do you think this would be a good approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have set the enhancement ot to CmcdObjectType.OTHER.
Thank you!
src/dash/models/DashManifestModel.js
Outdated
| } | ||
| if (realRepresentation.hasOwnProperty(DashConstants.DEPENDENCY_ID)) { | ||
| voRepresentation.dependentRepresentation = new Representation(); | ||
| voRepresentation.dependentRepresentation.id = realRepresentation[DashConstants.DEPENDENCY_ID].toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should handle the case when @dependencyId contains multiple @id valeus (as whitespace separated list):
@dsilhavy : What about added this to the capability checks, i.e. rejecting any Representation with more than one entry in this attribute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, great idea! We would like to reject the representation and log it in the console when @dependencyId contains multiple @id values as it would most likely indicate an error with the packager. Do you think this would be a good way to approach it?
|
@v-nova-romas Thank you for keeping this PR up to date. As this PR requires design changes in dash.js and I would like to carefully evaluate them, I need to move this PR to the milestone of v5.1. For v5 we have a working sample for LCEVC (https://reference.dashif.org/dash.js/nightly/samples/index.html#MPEG5Part2LCEVC). My apologies for the delay. |
- enhancement can be enabled/disabled - array of supported enhancement codecs can be extended
dsilhavy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional review comments:
- Please make sure to adjust the
index.d.tsto add newly added functions and classes - Consider adding a LCEVC testvector to our list of smoke vectors for functional tests: https://github.com/Dash-Industry-Forum/dash.js/blob/development/test/functional/config/test-configurations/streams/smoke.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please create a higher resolution export of this diagram. Ideally, can you also provide the source files for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a higher resolution export and source files for the diagram, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please create a higher resolution export of this diagram. Ideally, can you also provide the source files for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a higher resolution export and source files for the diagram, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please place all lcevc related sample files into a root folder lcevc located in samples. You can create additional subfolders in there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @dsilhavy ! Thank you for taking the time to review the PR!
I have moved all sample files to a root folder lcevc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please place all lcevc related sample files into a root folder lcevc located in samples. You can create additional subfolders in ther
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved all sample files to a root folder lcevc, thank you!
src/streaming/StreamProcessor.js
Outdated
| // Pass quality change on enhanced representation | ||
| enhancementStreamProcessor.prepareQualityChange(e); | ||
| } | ||
| else if (e.newRepresentation.mediaInfo.type !== e.oldRepresentation.mediaInfo.type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this check and the one after be more explicit? Like checking directly for enhancement. I don't quite the purpose here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check has been moved to new private function _prepareQualityChangeForEnhancementStreamProcessor and made more explicit, thank you!
Please let me know if the new check is suitable
src/streaming/StreamProcessor.js
Outdated
| } | ||
|
|
||
| function getAbrRepresentation() { | ||
| return representationController ? representationController.getCurrentRepresentation(false) : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this function if we already have getRepresentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve removed this function, thank you! Please let me know if this looks suitable.
| setCheckPlaybackQuality, | ||
| setInitSegmentRequired, | ||
| setLastInitializedRepresentationId, | ||
| setup, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to expose the setup method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a bit of logic in StreamProcessor.js _prepareQualityChangeForEnhancementStreamProcessor function that is responsible for starting and stopping the enhancement StreamProcessor when it is not needed.
In the current implementation, scheduleController.reset() is used to stop it. However, reset() clears the streamInfo of the scheduleController, and setup() is needed to set the streamInfo again.
I have tried the alternative of using startScheduleTimer and clearScheduleTimer methods of ScheduleController, but it seems we need to call resetInitialSettings() for it to work as expected, however it is also not exported.
Do you think this approach is suitable? Thank you!
src/streaming/utils/Capabilities.js
Outdated
| } | ||
|
|
||
| const enhancementCodecs = settings.get().streaming.enhancement.codecs; | ||
| if (settings.get().streaming.enhancement.enabled && basicConfiguration.codec.includes('video') && enhancementCodecs.some(cdc => basicConfiguration.codec.includes(cdc))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Use
Constantsfor type:Constants.Video - Do we need to limit this to
video?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the limitation to video, thank you!
- Move logic to a separate private function - Make check more explicit
- Remove getCurrentRepresentation overload - Add getCurrentDependentRepresentation
|
@v-nova-romas Thank you for addressing the comments. Please leave a short comment here once you are done with all your changes then I will have a look again and answer your remaining questions/comments. |
|
Thank you @dsilhavy ! I have gone through and made all the changes. Thank you so much for taking the time to review the PR! I would love to hear your thoughts on the updates whenever you have a chance. |
dsilhavy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the changes, left some remaining comments from my side. Can you please also merge the latest changes from the development branch into your PR
|
@v-nova-romas I see you are already working on the changes. Can you please leave a short note here again once you addressed all issues and the PR is ready for review again. |
|
@dsilhavy Thank you very much! I have addressed all comments and merged the latest changes from the |
|
Thanks @v-nova-romas this looks good to me. I will merge it now |
Adds MPEG-5 LCEVC scalable support into dash.js to enable playback of ABR profiles generated by LCEVC enhancement via a separate adaptation set.
This approach creates LCEVC representations which are dependent upon native codec representations (by using dependencyId). LCEVC enhancement representations are contained in a 2nd Adaptation Set and linked to the base representations in the 1st Adaptation Set. The outcome is the ability to play adaptive streaming content where one or more higher resolution profiles are generated by applying LCEVC enhancement to existing conventional profiles saving up to 70% bitrate compared to using conventional standalone native profiles.
There is a new section in dash.js samples for MPEG-5 Part 2 LCEVC where the scalable implementation is demonstrated.